Match CSS url() quoting rules when rewriting creative styles - #1106
Match CSS url() quoting rules when rewriting creative styles#1106prk-Jr wants to merge 11 commits into
Conversation
Treat a url() value as quoted only when a matching closing quote is present, and end a quoted value at that quote rather than at the first paren, so a value is rewritten the way a browser reads it. Derive srcset candidate state as the scan advances rather than from the candidate prefix at each comma.
The rewriter does not resolve CSS escapes, so a value carrying a backslash cannot be mapped to the resource the page will actually request; proxying the raw bytes points somewhere else. A raw newline, which preprocessing also produces from a carriage return or a form feed, makes the value a bad string the browser discards, so rewriting it proxies a URL that is never fetched. Both are now passed through untouched. Also fold an escaped CRLF into a single escaped newline when locating the end of a quoted string, and end the string at a form feed, so the extent matches what preprocessing produces.
Inserting the resolvability helper above css_string_end left that function's doc comment attached to the new helper.
|
Closes #1114 |
The scan bounded a value by the next quote and paren, so a span could fuse across declarations, a missing paren abandoned the rest of the input, an escape became a URL nobody requests, and a value ending at input was skipped. It also stepped back from a computed index, which slices a multi-byte character and aborts the guest. Reading with a tokenizer supplies both the extent and the resolved value, and leaves a malformed value — which a browser discards anyway — on its original bytes. Cover the other references a browser fetches: src(), a bare string candidate in image-set(), and an @import prelude, which takes one URL and reads later strings as media queries. A string counts as a URL only in those places, so font-family and content keep theirs, and an @import is a rule only where a top-level rule may start — the same token is data inside a declaration, in another prelude, and in a style attribute, which is not a stylesheet. The walk recurses per scope through upstream-supplied CSS, so it is bounded, and CSS past the bound is rejected rather than passed through below it. cssparser already built here as a transitive dependency.
… fix/creative-parser-bounds
A rewritten value went out as url() whatever it arrived as. For src() that changes what the browser does rather than where it points: an engine that ignores src() leaves the declaration inert, so emitting url() starts a request the origin never made. Keep the name and proxy the value. Read a var() fallback in the context around it, so a candidate written image-set(var(--c, "https://…") 1x) is proxied like the plain string it becomes. The propagation is deliberately narrow: substitution applies to declaration values, so the same fallback in a content value or an @import prelude stays untouched. A URL assembled from a separate custom-property declaration is left alone and documented — pairing the two is the cascade's work, not a rewriter's.
Bare-string references still went out wrapped in url(). That is valid in the places they are read, so nothing broke, but it is not valid once the same candidate is substituted into a src() argument, which has to stay a string. Re-emit each reference in the shape it arrived in and replace only the value. That makes the remaining src() gap fixable: src() takes a normal value list, so a var() there is substituted and its fallback is the string the engine ends up with. Walk it and rewrite the fallback where it sits, leaving both calls intact. url() is deliberately excluded, since an engine does not substitute inside it and a fallback there is never requested. The entry-point note claimed everything was re-emitted as url(), which the earlier src() change had already made untrue.
Both resolve to their fallback when the name is not set, so a string written there is a string the engine ends up with. Only var() was followed, which left image-set(env(--x, "https://...")) unproxied - and that is a supported feature reached by an unrecognised name, which is the case the fallback exists for, not a future one. Follow both, and pin that this reaches no further: a string in a gradient nested inside image-set is still not a URL.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This is a larger change than the description suggests: commits from a8d4e099e onward replace the hand-rolled CSS scanner with a cssparser-based grammar walk, adding url()/src(), image-set() bare-string candidates, @import preludes, and var()/env() fallback following. The token-level, property-agnostic design is the right shape for this problem — it covers URL-bearing properties nobody had to enumerate, while correctly leaving fragment-only references like filter:url(#blur) alone.
Two findings below. The depth cap admits a different nesting level depending on whether a URL is quoted, and the depth rejection is operationally invisible on the wire.
Both are prose-only: the wrench fix restructures a match across three arms and needs a companion test change, so it can't be expressed as a single contiguous suggestion.
Verified locally against the PR head: cargo fmt --all -- --check, clippy-fastly, test-fastly (2301 passed), test-axum, check-cloudflare, check-spin, and the cross-adapter parity suite (13 passed) all pass.
Three things I checked and am explicitly not raising, since each looked like a finding and turned out not to be:
@import layer(base) "url.css"looked like a missed egress leak, but the CSS grammar requires the URL first — that prefix form is invalid and browsers do not fetch it.&becoming&in rewritten<style>output is real, butgit show origin/mainconfirms the handler is byte-identical tomain. Pre-existing, not this PR.- The
cssparserdependency is free:cssparser 0.36.0is already inmain's lockfile vialol_html(Cargo.lock:2911). No new packages, and deleting the hand-rolled scanner makes the shipped WASM code sections roughly 19.7 KB smaller.
I also probed parse-error recovery specifically (bad-url tokens, unterminated strings, stray ), unmatched }, embedded NUL, CDO/CDC) and could not construct an input where a URL after the error point escapes rewriting.
Blocking
🔧 wrench
- Depth cap rejects at a different depth depending on whether the URL is quoted — see inline at
crates/trusted-server-core/src/creative.rs:93
❓ question
- Blanking the whole stylesheet is invisible to the client — see inline at
crates/trusted-server-core/src/creative.rs:159
Non-blocking
📝 note
- PR description no longer matches the change — see the Cross-cutting section below
Cross-cutting / body-level findings
- 📝 PR description no longer matches the change — The Summary and Changes tables describe a
css_string_endhelper and positional quote-scanning, which commita8d4e099edeleted. The "Verification against production CSS" section describes a differential run over a parser version that is no longer what ships, so that evidence no longer covers the code under review. Worth refreshing before merge so the squashed commit message describes what actually landed — and worth re-running the production-CSS differential against the grammar walk, since that check is genuinely valuable and currently attests to superseded code.
CI Status
At the time of review, checks on 441c831 were still running (the head is a fresh merge commit). No failures observed; the findings above are independent of CI.
- CodeQL: SKIPPED
- Analyze (actions): PASS
- Analyze (rust): PENDING
- Analyze (javascript-typescript): PENDING
- cargo test: PENDING (required)
- cargo test (axum native): PENDING
- cargo test (ts CLI, native): PENDING
- cargo test (cross-adapter parity): PENDING
- cargo check (cloudflare native + wasm32-unknown-unknown): PENDING
- cargo check/build/test (spin native + wasm32-wasip1): PENDING
- cargo fmt: PENDING (required)
- format-typescript: PENDING (required)
- format-docs: PENDING (required)
- vitest: PENDING
- prepare integration artifacts: PENDING
| /// block through a closure, so each nested slice would be re-read from its | ||
| /// start — quadratic in the input, which [`MAX_REWRITABLE_BODY_SIZE`] allows | ||
| /// to be 10 MB. Recursing stays linear and bounds the stack instead. | ||
| const MAX_CSS_NESTING_DEPTH: usize = 64; |
There was a problem hiding this comment.
🔧 wrench — The depth cap admits a different nesting level depending on whether the URL is quoted, and the difference silently discards the whole stylesheet.
Token::UnquotedUrl is a terminal token — it never recurses, so url(https://…) is rewritten at any depth. But url("https://…") arrives as Token::Function("url"), takes the CssToken::UrlFunction arm, hits the depth >= MAX_CSS_NESTING_DEPTH guard, and sets depth_exceeded, which discards the entire stylesheet. image-set() behaves the same way.
Reproduced at exactly MAX_CSS_NESTING_DEPTH (64) nested a{ blocks:
| input at depth 64 | result |
|---|---|
url(https://cdn.example/a.png) |
rewritten; stylesheet intact (459 bytes out) |
url("https://cdn.example/a.png") |
whole stylesheet discarded (287 bytes in, 0 bytes out) |
image-set("https://cdn.example/a.png" 1x) |
whole stylesheet discarded |
So the constant advertises 64 for one spelling of a URL and effectively 63 for another spelling of the same URL. A stylesheet is kept or dropped based on quoting, which is not a distinction the doc comment claims to make.
This passes CI because rewrite_style_urls_rewrites_at_the_deepest_supported_nesting (line 2428) pins only the unquoted form. Swapping that test's url(https://cdn.example/a.png) for the quoted url("https://cdn.example/a.png") fails on this branch, which is the cheapest way to confirm the above before changing anything.
The fix I would suggest is hoisting the single depth check above the match found so every recursing arm (UrlFunction, ImportPrelude, Block) shares one bound, rather than three separate guards that the terminal Url arm bypasses. Roughly:
// One bound, checked before any arm that recurses.
if matches!(
found,
CssToken::UrlFunction(_) | CssToken::ImportPrelude | CssToken::Block(..)
) && depth >= MAX_CSS_NESTING_DEPTH
{
self.depth_exceeded = true;
return;
}
match found {
// … arms with their individual depth checks removed …
}That still leaves url(unquoted) rewritable one level deeper than url("quoted"), since only one of them opens a scope — so whichever shape you choose, it is worth extending the depth test to cover the quoted and image-set() forms so the bound the constant advertises is the bound every spelling actually gets.
Apply manually — can't be auto-applied as a suggestion because the change spans three arms of the match plus a companion test edit outside this hunk.
| let mut parser = cssparser::Parser::new(&mut input); | ||
| rewriter.walk(&mut parser, 0, BareStringUrls::Never); | ||
| if rewriter.depth_exceeded { | ||
| log::warn!("Rejecting a stylesheet nested past the supported depth"); |
There was a problem hiding this comment.
❓ question — Is a zero-byte 200 the right thing to put on the wire when the depth cap trips?
To be clear on the part I agree with: fail-closed over leaking is the right call, and the reasoning in this doc comment for why partial rewriting would defeat the purpose is sound. My question is only about what the client ends up seeing.
Traced through proxy.rs:666, a proxied text/css response that trips the cap is returned as HTTP 200 with an empty body — indistinguishable from a stylesheet the origin legitimately served as empty. Confirmed by driving CreativeCssProcessor directly: 1026 bytes in, 0 bytes out, no error.
The log::warn! on the next line is the only signal, and it is edge-side. A publisher seeing an unstyled creative has nothing in the response that distinguishes "we rejected this stylesheet" from "upstream sent nothing", which makes this a hard failure to attribute in production.
Would an error status — letting finalize_proxied_response decide, the way the oversized-body path already does — be preferable here? Or, if 200-with-empty-body is deliberate, a distinguishing response header would at least make the rejection attributable from a HAR capture.
Not arguing this is likely to fire: I verified that realistic nesting (@media + @supports + @layer + @container + image-set) sits nowhere near 64, and that 500 sibling blocks are fine. This is about diagnosability when it does.
2 participants
Summary
url()rewriter parsed quoting by position rather than by matching delimiters, so it disagreed with how a browser reads the same declaration. Values were rewritten in ways the browser would not, and some were left unrewritten.). The rewriter ended the value at the first)afterurl(, which truncated such values and left the intended URL unproxied.split_srcset_candidatesre-derived per-candidate facts from the whole candidate prefix at each comma. Those facts belong to the candidate, so they are now tracked as the scan advances.Changes
crates/trusted-server-core/src/creative.rsrewrite_style_urls: treat a value as quoted only when a matching closing quote is present; locate the closing paren after the closing quote so a quoted value keeps an inner)crates/trusted-server-core/src/creative.rscss_string_endhelper: finds the quote that closes a CSS string, honoring backslash escapes and treating a raw newline as ending the stringcrates/trusted-server-core/src/creative.rssplit_srcset_candidates: derive candidate scheme and whitespace state as the scan advances instead of from the candidate prefix at each comma; corrected the doc note, which described behavior the function did not havecrates/trusted-server-core/src/creative.rsdata:casesBehavior
url("https://cdn.example/a)b.png")url("https://cdn.example/plain.png")url('/local/a.png")url("https://cdn.example/a+ newline +b)srcset="data:...;base64,,,,, 1x, /b.png 2x"Unquoted values still end at the first
), matching the CSS url-token rule.Verification against production CSS
Captured the 18 unique stylesheets served through a local
fastly compute servesession and ran both themainand branch versions ofrewrite_css_bodyover the identical corpus. Output is byte-identical on all 18, and both leave every file unmodified — the values there aredata:URIs, whichto_absdeclines. No change to real traffic.Closes
Closes #1114
Test plan
cargo test-fastly && cargo test-axum(alsotest-cloudflare,test-spin)cargo clippy-fastly && cargo clippy-axum(alsoclippy-cloudflare,clippy-cloudflare-wasm,clippy-spin-native,clippy-spin-wasm,trusted-server-cli)cargo fmt --all -- --checkcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!) — no logging added